Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement delete release attachments and update release attachments' name #14130

Merged
merged 11 commits into from
Mar 22, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 23, 2020

image

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 23, 2020
@lunny lunny added this to the 1.14.0 milestone Dec 23, 2020
@silverwind
Copy link
Member

silverwind commented Dec 23, 2020

Fixes #12963

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2020
@silverwind
Copy link
Member

Some improvements you can pull in for those lists
diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl
index 75a651aaf..0e15b4de5 100644
--- a/templates/repo/release/list.tmpl
+++ b/templates/repo/release/list.tmpl
@@ -134,37 +134,44 @@
 							<div class="markdown desc">
 								{{Str2html .Note}}
 							</div>
 							<div class="ui accordion download">
-								<h2 class="title {{if eq $idx 0}}active{{end}} df ac mb-0">
-									{{svg "octicon-triangle-right" 14 "dropdown icon"}}
+								<h2 class="title active df ac mb-0">
+									{{svg "octicon-triangle-right" 16 "dropdown icon"}}
 									{{$.i18n.Tr "repo.release.downloads"}}
 								</h2>
-								<div class="content {{if eq $idx 0}}active{{end}}">
+								<div class="content active">
 									<ul class="list">
 										{{if $.Permission.CanRead $.UnitTypeCode}}
-											<li>
-												<a class="archive-link" data-url="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow"><strong>{{svg "octicon-file-zip" 16 "mr-2"}}{{$.i18n.Tr "repo.release.source_code"}} (ZIP)</strong></a>
+											<li class="df ac p-3">
+												<a class="archive-link df ac f1 bold" data-url="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow">
+													{{svg "octicon-file-zip" 16 "ml-2 mr-3"}}
+													{{$.i18n.Tr "repo.release.source_code"}} (ZIP)
+												</a>
 											</li>
-											<li>
-												<a class="archive-link" data-url="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.tar.gz"><strong>{{svg "octicon-file-zip" 16 "mr-2"}}{{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ)</strong></a>
+											<li class="df ac p-3">
+												<a class="archive-link df ac f1 bold" data-url="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.tar.gz">
+													{{svg "octicon-file-zip" 16 "ml-2 mr-3"}}
+													{{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ)
+												</a>
 											</li>
 										{{end}}
 										{{if .Attachments}}
 											{{range .Attachments}}
-												<li>
-													<span class="ui text middle aligned right">
+												<li class="df ac p-3">
+													<a class="df f1 bold" target="_blank" rel="noopener noreferrer" href="{{.DownloadURL}}">
+														{{svg "octicon-package" 16 "ml-2 mr-3"}}
+														{{.Name}}
+													</a>
+													<div class="stats df ac">
 														<span class="ui text grey">{{.Size | FileSize}}</span>
 														<span class="poping up" data-content="{{$.i18n.Tr "repo.release.download_count" (.DownloadCount | PrettyNumber)}}">
 															{{svg "octicon-info"}}
 														</span>
 														<a class="ui mini compact red button delete-button remove-rel-attach" data-url="{{$.RepoLink}}/releases/attachments/remove" data-file="{{.UUID}}">
 															{{$.i18n.Tr "remove"}}
 														</a>
 													</span>
-													<a target="_blank" rel="noopener noreferrer" href="{{.DownloadURL}}">
-														<strong><span class="ui image" title='{{.Name}}'>{{svg "octicon-package" 16 "mr-2"}}</span>{{.Name}}</strong>
-													</a>
 												</li>
 											{{end}}
 										{{end}}
 									</ul>
diff --git a/web_src/less/_repository.less b/web_src/less/_repository.less
index c0fd13c09..7b4012f13 100644
--- a/web_src/less/_repository.less
+++ b/web_src/less/_repository.less
@@ -1701,20 +1701,27 @@
             }

             .list {
               padding-left: 0;
-              border-top: 1px solid var(--color-secondary);
+              background: var(--color-box-body);
+              border: 1px solid var(--color-secondary);
+              border-radius: var(--border-radius);

               li {
                 list-style: none;
-                display: block;
-                padding-top: 8px;
-                padding-bottom: 8px;
-                border-bottom: 1px solid var(--color-secondary);

-                a > .text.right {
-                  margin-right: 5px;
+                .stats * + * {
+                  margin-left: .5rem;
                 }

+                .delete-button {
+                  margin-top: -4px;
+                  margin-bottom: -4px;
+                }
+              }

+              li + li {
+                border-top: 1px solid var(--color-secondary);
               }
             }
           }

image

@lunny
Copy link
Member Author

lunny commented Dec 24, 2020

@silverwind Please help to just push to my branch, I think I have allowed that.

@noerw
Copy link
Member

noerw commented Dec 24, 2020

also fixes #677

@CirnoT
Copy link
Contributor

CirnoT commented Dec 24, 2020

Don't like the delete functionality being visible directly, it should follow standard we have set for attachments where list of files is shown with removal ability on edit.

@silverwind
Copy link
Member

silverwind commented Dec 24, 2020

Yeah, I generally also feel the deletion should be done via edit function and does not need to be so promiment, e.g. prepopulate dropzone with attachments (even thought I don't really like dropzone's UI and I think a straight vertical list of files on the edit view would be better).

@CirnoT
Copy link
Contributor

CirnoT commented Dec 24, 2020

and I think a straight vertical list of files on the edit view would be better).

Please, this so much. dropzone is awful.

In this case however populating dropzone is better option since it's already present there for upload of new files.

@silverwind
Copy link
Member

Yes, we should replace dropzone long-term, but short term I guess it has to be done that way.

@lunny
Copy link
Member Author

lunny commented Dec 27, 2020

I will change the PR to update the Edit page but not the view page.

@lunny lunny modified the milestones: 1.14.0, 1.15.0 Jan 27, 2021
@lunny lunny force-pushed the lunny/delete_release branch 3 times, most recently from 7517089 to 9b679be Compare March 17, 2021 09:59
@lunny lunny changed the title WIP: Implement delete release attachment Implement delete release attachment Mar 17, 2021
@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Mar 17, 2021
services/release/release.go Outdated Show resolved Hide resolved
web_src/js/index.js Show resolved Hide resolved
@lunny lunny changed the title Implement delete release attachment Implement delete release attachments and update release attachments' name Mar 18, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 19, 2021
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this fails when isCreate is set to true in UpdateReleaseOrCreatReleaseFromTag

Add the following test services/release/release_test.go TestRelease_Update to recreate


	// Test create release
	release = &models.Release{
		RepoID:       repo.ID,
		PublisherID:  user.ID,
		TagName:      "v1.1.2",
		Target:       "master",
		Title:        "v1.1.2 is released",
		Note:         "v1.1.2 is released",
		IsDraft:      false,
		IsPrerelease: false,
		IsTag:        false,
	}

	tagName := release.TagName
	
	assert.NoError(t, UpdateReleaseOrCreatReleaseFromTag(user, gitRepo, release, nil, nil, nil,  true))
	release, err = models.GetReleaseByID(release.ID)
	assert.NoError(t, err)
	assert.Equal(t, tagName, release.TagName)

Result:

--- FAIL: TestRelease_Update (2.46s)
    release_test.go:156:
                Error Trace:    release_test.go:156
                Error:          Received unexpected error:
                                release tag does not exist [id: 0, tag_name: ]
                Test:           TestRelease_Update
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x28 pc=0x176a1fa]

@lunny
Copy link
Member Author

lunny commented Mar 22, 2021

image

Can you vertically center text and button? Should be just adding df ac classes on parent.

Done.

Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it works great! Thanks for adding relevant unit tests.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 22, 2021
@lunny lunny merged commit 8567cba into go-gitea:master Mar 22, 2021
@lunny lunny deleted the lunny/delete_release branch March 22, 2021 16:09
kdumontnu pushed a commit to AllSpiceIO/gitea that referenced this pull request Apr 29, 2021
…name (go-gitea#14130)

* Implement delete release attachment

* Add attachments on release edit page

* Fix bug

* Finish del release attachments

* Fix frontend lint

* Fix tests

* Support edit release attachments

* Added tests

* Remove the unnecessary parameter isCreate from UpdateReleaseOrCreatReleaseFromTag

* Rename UpdateReleaseOrCreatReleaseFromTag to UpdateRelease

* Fix middle align
@6543 6543 added type/enhancement An improvement of existing functionality backport/v1.14 backport/done All backports for this PR have been created and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels May 2, 2021
@6543
Copy link
Member

6543 commented May 2, 2021

-> #15666

techknowlogick pushed a commit that referenced this pull request May 3, 2021
…name (#14130) (#15666)

* Implement delete release attachment

* Add attachments on release edit page

* Fix bug

* Finish del release attachments

* Fix frontend lint

* Fix tests

* Support edit release attachments

* Added tests

* Remove the unnecessary parameter isCreate from UpdateReleaseOrCreatReleaseFromTag

* Rename UpdateReleaseOrCreatReleaseFromTag to UpdateRelease

* Fix middle align

Co-authored-by: Lunny Xiao <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants